-
Notifications
You must be signed in to change notification settings - Fork 45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
solana: Add SPL multisig support #568
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall looks good, but most of the code is duplication from the original versions. I wonder if we could refactor to share the logic?
4e93386
to
6f91d71
Compare
I tried refactoring out the common structs and code logic to form a "clean" solution (the latest WIP commit). However, with this, we break the ABI as the accounts are now passed in differently for the original If I were to preserve the ABI, there is minimal/no refactoring possible for the structs. This makes it difficult to use refactored code logic as there is no common shared struct that can be used; instead every required account has to be passed explicitly. Although I prefer the "clean" solution as it reduces code duplication and makes the Ideally, we'd want to refactor code without breaking the ABI. |
4c6f733
to
70dd3c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only nits.
solana/programs/example-native-token-transfers/src/instructions/initialize.rs
Outdated
Show resolved
Hide resolved
solana/programs/example-native-token-transfers/src/instructions/initialize.rs
Outdated
Show resolved
Hide resolved
solana/programs/example-native-token-transfers/src/instructions/initialize.rs
Outdated
Show resolved
Hide resolved
solana/programs/example-native-token-transfers/src/instructions/release_inbound.rs
Outdated
Show resolved
Hide resolved
solana/programs/example-native-token-transfers/src/instructions/release_inbound.rs
Outdated
Show resolved
Hide resolved
solana/programs/example-native-token-transfers/src/instructions/release_inbound.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, only nits. If you decide to address them, I promise I'll be quick with rubberstamping those changes this time around.
solana/programs/example-native-token-transfers/src/instructions/initialize.rs
Outdated
Show resolved
Hide resolved
solana/programs/example-native-token-transfers/src/instructions/release_inbound.rs
Outdated
Show resolved
Hide resolved
solana/programs/example-native-token-transfers/src/instructions/release_inbound.rs
Outdated
Show resolved
Hide resolved
solana/programs/example-native-token-transfers/src/instructions/release_inbound.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two more remarks.
solana/programs/example-native-token-transfers/src/spl_multisig.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This PR adds:
initialize_multisig
andrelease_inbound_multisig_mint
to act as multisig variants ofinitialize
andrelease_inbound_mint
respectively